Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JBLOGGING-189 Include originating element when creating files with Filer #119

Merged
merged 1 commit into from
Aug 15, 2024
Merged

JBLOGGING-189 Include originating element when creating files with Filer #119

merged 1 commit into from
Aug 15, 2024

Conversation

marko-bekhta
Copy link
Contributor

https://issues.redhat.com/browse/JBLOGGING-189

and here's an example based on Hibernate ORM build:

Without the patch:

> Task :hibernate-core:compileJava
Build cache key for task ':hibernate-core:compileJava' is 9179d4b00e8baddeb848f63a0795416c
Task ':hibernate-core:compileJava' is not up-to-date because:
  Input property 'stableSources' file /home/marko/development/projects/opensource/hibernate-orm/hibernate-core/src/main/java/org/hibernate/cache/CacheException.java has changed.
Created classpath snapshot for incremental compilation in 0.004 secs.
Full recompilation is required because the generated resource 'org/hibernate/dialect/DialectLogging.i18n.properties in CLASS_OUTPUT' must have exactly one originating element, but had 0. Analysis took 0.073 secs.
Compiling with toolchain '/home/marko/.sdkman/candidates/java/21.0.3'.
Log level has changed, stopping idle worker daemon with out-of-date log level.

BUILD SUCCESSFUL in 1m 4s
110 actionable tasks: 9 executed, 101 up-to-date

With the patch:

> Task :hibernate-core:compileJava
Build cache key for task ':hibernate-core:compileJava' is 26a02c1fbcf88249babe7df8000b2289
Task ':hibernate-core:compileJava' is not up-to-date because:
  Input property 'stableSources' file /home/marko/development/projects/opensource/hibernate-orm/hibernate-core/src/main/java/org/hibernate/cache/CacheException.java has changed.
Created classpath snapshot for incremental compilation in 0.004 secs.
Compiling with toolchain '/home/marko/.sdkman/candidates/java/21.0.3-amzn'.
Log level has changed, stopping idle worker daemon with out-of-date log level.

Incremental compilation of 544 classes completed in 9.531 secs.
Class dependency analysis for incremental compilation took 0.04 secs.


BUILD SUCCESSFUL in 40s
110 actionable tasks: 9 executed, 101 up-to-date

@marko-bekhta marko-bekhta force-pushed the feat/JBLOGGING-189-add-originatingg-elements branch from 335199a to 3896e0a Compare August 2, 2024 07:56
Sanne
Sanne reviewed Aug 15, 2024
View reviewed changes
Copy link
Contributor

@Sanne Sanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome - left some minor, optional suggestions.
(It's not my place to approve)

processor/src/main/java/org/jboss/logging/processor/generator/model/ClassModel.java
private final Filer filer;

private JFilerOriginatingElementAware(Element originatingElement, Filer filer) {
this.originatingElement = originatingElement;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd check here for these elements to not be null?
Otherwise it fails later and it would get trickier to narrow down

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to propose this to happen slightly differently. I'd propose throwing a ProcessingException as it can take the element as an argument and potentially fail with a better message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok 😃, so maybe something like this ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. I know these are highly unlikely, but may as well have as much information as possible if they are thrown :)

processor/src/main/java/org/jboss/logging/processor/generator/model/ClassModel.java
@@ -362,4 +368,26 @@ private JExpr determineLocale(final String locale, final JType localeType) {

return initializer;
}

private static class JFilerOriginatingElementAware extends JFiler {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to add a little javadoc, or some comments, to explain to other maintainers why this is useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look and for the suggestions 😃🎉
Applied the changes and pushed an updated version 😃

@marko-bekhta marko-bekhta force-pushed the feat/JBLOGGING-189-add-originatingg-elements branch from 3896e0a to ee6374d Compare August 15, 2024 14:27
@marko-bekhta marko-bekhta force-pushed the feat/JBLOGGING-189-add-originatingg-elements branch from ee6374d to 8bd9467 Compare August 15, 2024 15:27
@jamezp jamezp merged commit be1c51f into jboss-logging:main Aug 15, 2024
9 checks passed
@marko-bekhta
Copy link
Contributor Author

Hey @jamezp 👋🏻 😃

Any chance you could do another release for jboss-logging-tools including the patch from this PR? We'd want to use it in ORM to improve (hopefully) the incremental builds. Thanks!

@jamezp
Copy link
Member

jamezp commented Oct 4, 2024

@marko-bekhta I guess I'm not as caught up from PTO as I thought :) Yes, next week I will get a release out.

@marko-bekhta
Copy link
Contributor Author

Thanks 😃 🎉

@jamezp
Copy link
Member

jamezp commented Oct 11, 2024

Sorry for the delay @marko-bekhta, but done https://github.com/jboss-logging/jboss-logging-tools/releases/tag/3.0.2.Final

@marko-bekhta
Copy link
Contributor Author

Thanks, James! 😃
Now to update it in ORM 🤞🏻😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sanne Sanne Sanne left review comments

@jamezp jamezp jamezp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marko-bekhta @jamezp @Sanne